feat(banner): add x for closing#9008
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview The site Reviewed by Cursor Bugbot for commit 64f758c. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9008 +/- ##
==========================================
+ Coverage 74.58% 74.61% +0.03%
==========================================
Files 100 100
Lines 8801 8801
Branches 324 324
==========================================
+ Hits 6564 6567 +3
+ Misses 2233 2230 -3
Partials 4 4 ☔ View full report in Codecov by Harness. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📦 Build Size ComparisonSummary
Changes🔄 Modified Routes (4)
|
bjohansebas
left a comment
bjohansebas
left a comment
There was a problem hiding this comment.
I think that if we ever update the banner and a user previously dismissed it, we should show it to them again
|
there are a flash humm not fan of it maybe not display it by default. also what will happen on doc-kit since it's already flash when fetching site.json |
|
To avoid flash, can we dismiss it by default and show only if the value is not set? Screen recording of flash on reload post dismissnodejs-banner-flash-on-reload-post-dismiss.mov |
|
Fixes: #6292 |
|
@trivikr either we flash on the way in, or the way out (fwiw we can make it a fade) The way I see it, if we fade on the way in, we fade for all website viewers. If we fade on the way out, we only fade for a small subset of viewers who have opted in to hiding this banner |
This comment was marked as outdated.
This comment was marked as outdated.
|
With assistance from openai:gpt-5.6-sol, I attempted going prehydration in #9009. Would that help fix the flashing issue? |
|
No, unfortunately. While I appreciate the time and tokens spent trying to help, there's several layers to how we render our components currently, including:
Attaching a In Would that satisfy your concern? |
|
I've manually re-deployed the previous commit to match Vercel's branch preview with the correct commit. |
Sure. |
++ I think I would actually prefer that we do fade in everywhere, for consistency and to clear the way for this potentially being fully dynamic in the future instead of needing a site rebuild every time it is changed. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 64f758c. Configure here.
| items-center | ||
| justify-center | ||
| gap-2; | ||
| } |
There was a problem hiding this comment.
Banner animation missing overflow clip
Medium Severity
The enter animation animates grid-template-rows from 0fr to 1fr and bannerContent already has min-h-0, but neither .banner nor .bannerContent sets overflow-hidden. Without clipping, banner text and the close control still paint outside the collapsing box while opacity rises, so content bleeds over the navbar and can intercept clicks during the delay and fade-in.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 64f758c. Configure here.
| const [open, setOpen] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (banner) { | ||
| // eslint-disable-next-line @eslint-react/set-state-in-effect | ||
| setOpen(localStorage.getItem(STORAGE_KEY) !== banner.text); | ||
| } | ||
| }, [banner]); |
There was a problem hiding this comment.
| const [open, setOpen] = useState(false); | |
| useEffect(() => { | |
| if (banner) { | |
| // eslint-disable-next-line @eslint-react/set-state-in-effect | |
| setOpen(localStorage.getItem(STORAGE_KEY) !== banner.text); | |
| } | |
| }, [banner]); | |
| const [open, setOpen] = useState(() => !!banner && localStorage.getItem(STORAGE_KEY) !== banner.text); |
Can we avoid the effect + lint disable here?
There was a problem hiding this comment.
No, unfortunately, Next.js attempts to evaluate that on the server side, causing an error
There was a problem hiding this comment.
Ah, surely we can just check for window in that then? Or will the state not get re-evaluated on the client?
| const [open, setOpen] = useState(false); | |
| useEffect(() => { | |
| if (banner) { | |
| // eslint-disable-next-line @eslint-react/set-state-in-effect | |
| setOpen(localStorage.getItem(STORAGE_KEY) !== banner.text); | |
| } | |
| }, [banner]); | |
| const [open, setOpen] = useState(() => !!banner && typeof window !== 'undefined' && window.localStorage.getItem(STORAGE_KEY) !== banner.text); |
There was a problem hiding this comment.
If I understand correctly, that'll cause a hydration mismatch, since on the server, it'll be false, and on the client, it'll be either false or true.
Makes the banner close-able.
Note: This just updated
ui-components, and doesn't touch Next.js + cookie handling